-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #11644: Fix broken dotty.tools.io.Path#parent #11662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The failure in |
else path match { | ||
case "" | "." => Directory("..") | ||
case _ => Directory(".") | ||
} | ||
case parent => Directory(parent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed a case that is still broken because it takes this case parent
path:
scala> Path("../..").parent
val res0: dotty.tools.io.Directory = .. // should be ../../..
@@ -127,7 +127,15 @@ class Path private[io] (val jpath: JPath) { | |||
* @return The path of the parent directory, or root if path is already root | |||
*/ | |||
def parent: Directory = jpath.normalize.getParent match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's best to avoid normalize
altogether.
Consider this case:
- working directory is
/src/dotty
foo
is a symlink to/tmp/foo
Before this PR:
scala> Path("foo/../bar").parent
val res0: dotty.tools.io.Directory = foo/../bar
scala> res0.jpath.toRealPath()
val res1: java.nio.file.Path = /tmp/bar
With this PR:
scala> Path("foo/../bar").parent
val res0: dotty.tools.io.Directory = .
scala> res0.jpath.toRealPath()
val res1: java.nio.file.Path = /src/dotty
Scala 2:
scala> Path("foo/../bar").parent
val res0: scala.reflect.io.Directory = foo/..
scala> res0.jfile.getCanonicalPath()
val res1: String = /tmp
readlink / realpath:
/src/dotty$ readlink -f foo/../bar/..
/tmp
/src/dotty$ realpath foo/../bar/..
/tmp
/src/dotty$ realpath -L foo/../bar/..
/src/dotty
@griggt Thanks for fixing the files in directory |
@griggt what else do you need to finish on this PR? |
Shell wildcards are not expanded when quoted, and thus the contents of the temporary output directories were never being cleared. One of the tests in bootstrapCmdTests had to be fixed as a result, since it was relying on a TASTy file created by a previous test still existing in the $OUT directory after having been cleared.
The previous implementation wrongly assumed that if `jpath.normalize.getParent` returns null, then `jpath` must be a root. However the null in this case really only means that no name elements remain after normalization and the call to `getParent`. Since redundant name elements such as "." and ".." may be removed by `normalize`, and `getParent` may simply remove the last name element, there are cases other than roots to consider, such as the current directory. Some examples of broken behavior prior this commit: scala> Path("./foo.txt").parent val res0: dotty.tools.io.Directory = ./foo.txt // should be Directory(.) scala> Path("foo.txt").parent val res1: dotty.tools.io.Directory = foo.txt // should be Directory(.) scala> Path(".").parent val res4: dotty.tools.io.Directory = . // should be Directory(..) The changes here are based in part on the Scala 2.13 implementation of scala.reflect.io.Path#parent Fixes scala#11644
Several CI jobs (test_windows_fast, test_windows_full, scaladoc/build, scaladoc/community-docs) are failing due to a Bintray / JCenter outage: https://status.bintray.com/incidents/9n84kbm0bjh0 https://github.com/lampepfl/dotty/actions/runs/656300195 |
Calling java.nio.file.Path#normalize may result in resolving to a different path than intended, as in the case where the given path contains a `..` element and the preceding name is symbolic link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[test_non_bootstrapped]
[test_windows_full]
Fixes #11644